Conversation
eddeee888:oss:verify
🦋 Changeset detectedLatest commit: 0a09764 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
`getGraphQLObjectTypeResolversToGenerate` (both 'smart' and 'fast' modes) treated a mapper whose type resolves to the TS error type - e.g. one aliasing an import that doesn't exist yet, such as a Prisma client not generated on a fresh install - the same as a mapper that genuinely has zero fields. Every schema field then took the "field is missing from mapper" branch and got a stub, silently overwriting hand-maintained resolvers with broken Promise<void> implementations. Add isNodeTypeUnresolved(), which distinguishes the TS error type (intrinsicName === 'error') from a real `any` (intrinsicName === 'any') and from a genuinely empty object type (not an intrinsic type at all). When a mapper's declaration node is unresolved, skip resolver generation for that schema type - leaving existing resolvers untouched - and log a warning naming the mapper so the real problem (the unresolved import) surfaces instead of being masked. eddeee888:oss:fix
eddeee888
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Fixes #446:
fixObjectTypeResolvers(both'smart'and'fast'modes) treated a mapper whose aliased type resolves to the TS error type (e.g. an import to a not-yet-generated module, such as a Prisma client on a fresh install) the same as a mapper that genuinely has zero fields — generating a "resolver is required because ... does not [exist on the mapper]" stub for every field of the schema type and silently overwriting hand-maintained resolvers.Fixes #446
Fix chosen
Of the options discussed, went with the narrowest, targeted fix: detect the unresolved mapper at the whole-declaration level and skip generation for it entirely (rather than per-property detection, or hard-failing the codegen run).
getGraphQLObjectTypeResolversToGenerate.spec.ts— the original checkpoint test proving the bug (now passing), extended to also cover'fast'mode and the new warning, plus a regression test confirming a genuinely-empty mapper still stubs as before.getNodePropertyMap.ts— addsisNodeTypeUnresolved(), which distinguishes the TS error type (intrinsicName === 'error') from a realany(intrinsicName === 'any') and from a genuinely empty object type (not an intrinsic type at all).getGraphQLObjectTypeResolversToGenerate.ts— in both'smart'and'fast'modes, when the mapper's declaration node is unresolved, skip resolver generation for that schema type (leaving existing resolvers untouched) andlogger.warnnaming the mapper, so the real problem surfaces instead of being masked.nx build/test/lint typescript-resolver-filesall pass.Note
#455 from the original reporter independently implements essentially the same fix (same
intrinsicName === 'error'detection). This PR was written independently against theeddeee888:oss:verifycheckpoint rather than building on top of that PR.